Automated Test: fix/handle-collective-multiple-host-destinations #359
Conversation
📝 WalkthroughWalkthroughThis PR refactors destination calendar handling from a single value to an array format across the codebase. Multiple calendar services are updated to support credential-based calendar selection, event management now handles multi-calendar scenarios, and booking workflows pass destination calendars as arrays throughout API endpoints and handlers. Changes
Sequence DiagramsequenceDiagram
participant Client
participant EventManager
participant CalendarManager
participant CalendarService
participant CredentialDB
Client->>EventManager: createAllCalendarEvents(evt with destCalendar[])
EventManager->>EventManager: For each destination calendar
alt Has credentialId
EventManager->>CredentialDB: Find credential by ID
CredentialDB-->>EventManager: Credential
EventManager->>CalendarManager: createEvent(calEvent, credentialId)
else No credentialId
EventManager->>CredentialDB: Find credentials by integration
CredentialDB-->>EventManager: Credentials[]
EventManager->>CalendarManager: createEvent(calEvent, credentialId)
end
CalendarManager->>CalendarManager: Extract mainHostDestinationCalendar
CalendarManager->>CalendarService: createEvent(calEvent, credentialId)
CalendarService->>CalendarService: Match calendar by credentialId
CalendarService-->>CalendarManager: NewCalendarEventType
CalendarManager-->>EventManager: EventResult with externalId
EventManager-->>Client: Event created across all calendars
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/trpc/server/routers/viewer/organizations/create.handler.ts (1)
149-158:⚠️ Potential issue | 🔴 CriticalCritical: Inverted slug/requestedSlug logic — organizations without billing will have no slug.
The codebase pattern (seen in
update.handler.tsandpublish.handler.ts) is:
- When
IS_TEAM_BILLING_ENABLEDis true: userequestedSlugin metadata for payment approval flow- When
IS_TEAM_BILLING_ENABLEDis false: useslugdirectly (no approval needed)This code inverts that logic:
- When billing is enabled: sets both
slugandrequestedSlug(bypasses approval, redundant)- When billing is disabled: sets neither (organization created with no slug, breaking functionality)
The conditions should use
!IS_TEAM_BILLING_ENABLEDforslugandIS_TEAM_BILLING_ENABLEDforrequestedSlug.packages/trpc/server/routers/viewer/bookings/confirm.handler.ts (1)
367-367:⚠️ Potential issue | 🟡 MinorPre-existing bug: operator precedence causes
messageto always be"confirmed".Due to
+having higher precedence than?, the expression"Booking " + confirmedis evaluated first, producing a non-empty string (always truthy), so the ternary always resolves to"confirmed".🐛 Proposed fix
- const message = "Booking " + confirmed ? "confirmed" : "rejected"; + const message = "Booking " + (confirmed ? "confirmed" : "rejected");packages/lib/CalendarService.ts (1)
125-125:⚠️ Potential issue | 🔴 Critical
createEventsignature does not match theCalendarinterface requirement.The
Calendarinterface (packages/types/Calendar.d.ts) definescreateEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType>, butBaseCalendarService.createEvent(line 125) only accepts(event: CalendarEvent). SinceBaseCalendarService implements Calendar, the signatures must match. Add the missingcredentialId: numberparameter to the method signature.Proposed fix
- async createEvent(event: CalendarEvent): Promise<NewCalendarEventType> { + async createEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType> {packages/features/bookings/lib/handleNewBooking.ts (1)
398-398:⚠️ Potential issue | 🟡 MinorRemove leftover
console.logdebug statement.This line logs potentially sensitive calendar busy-time data to stdout. The file already uses a structured logger (
logon line 94). This appears to be a debug artifact.🧹 Suggested fix
- console.log("calendarBusyTimes==>>>", bufferedBusyTimes); + log.debug("calendarBusyTimes", JSON.stringify(bufferedBusyTimes));Or remove it entirely if not needed.
🤖 Fix all issues with AI agents
In `@apps/web/playwright/webhook.e2e.ts`:
- Line 249: The handlers return inconsistent types for destinationCalendar:
confirm.handler.ts (BOOKING_REJECTED) returns [] while handleNewBooking.ts
(BOOKING_CREATED/BOOKING_REQUESTED) returns null; pick a single canonical shape
(either always array or always null), update both handlers (confirm.handler.ts
and handleNewBooking.ts) to return that shape for destinationCalendar
consistently, and adjust the tests in webhook.e2e.ts to expect the unified value
for all webhook event tests (update cases for BOOKING_REJECTED, BOOKING_CREATED,
BOOKING_REQUESTED accordingly).
In `@packages/app-store/googlecalendar/lib/CalendarService.ts`:
- Around line 145-154: The patch call in calendar.events.patch uses
selectedCalendar directly which can be undefined; update the calendarId passed
to calendar.events.patch (in CalendarService.ts where calendar.events.patch is
invoked) to use the same fallback as the insert call by changing calendarId:
selectedCalendar to calendarId: selectedCalendar || "primary" so it defaults to
"primary" when no matching destinationCalendar entry exists.
- Around line 315-317: The current assignment to calendarId uses a tautological
find: when externalCalendarId is falsy the predicate cal.externalId ===
externalCalendarId can never match, making the find dead code; change the logic
in CalendarService where calendarId is computed so that if externalCalendarId is
provided use it, otherwise select a fallback by searching
event.destinationCalendar for a suitable default (e.g., first calendar or one
matching another criterion) before falling back to defaultCalendarId—update the
expression that references externalCalendarId,
event.destinationCalendar?.find(...), and defaultCalendarId to perform the find
only when externalCalendarId is falsy and the list exists.
- Around line 254-256: selectedCalendar is being computed tautologically by
comparing cal.externalId to externalCalendarId, so when externalCalendarId is
falsy the find always returns undefined and calendar.events.update gets
undefined; change the else-branch to locate the externalId from
event.destinationCalendar (e.g., find by credentialId or fall back to the first
destinationCalendar's externalId) — update the expression that assigns
selectedCalendar (and any related logic used later in calendar.events.update) to
mirror the createEvent pattern: use externalCalendarId if present, otherwise use
event.destinationCalendar.find(cal => cal.credentialId ===
credentialId)?.externalId ?? event.destinationCalendar[0]?.externalId.
In `@packages/core/EventManager.ts`:
- Around line 117-121: The code destructures mainHostDestinationCalendar from
evt.destinationCalendar and then accesses
mainHostDestinationCalendar.integration without a null check, causing a
potential TypeError; update the condition in EventManager (where evt,
destinationCalendar, mainHostDestinationCalendar, and MeetLocationType are used)
to guard for undefined by using optional chaining or an explicit check (e.g.,
mainHostDestinationCalendar?.integration !== "google_calendar" or
mainHostDestinationCalendar && mainHostDestinationCalendar.integration !==
"google_calendar") so that when destinationCalendar is empty or undefined it
safely falls back to the daily integration.
- Around line 504-510: Rename the misspelled local variable calenderExternalId
to calendarExternalId inside the loop that iterates over calendarReference (the
block that destructures bookingRefUid and bookingExternalCalendarId) and update
all subsequent usages of that variable within the same function (every place
that currently reads or assigns calenderExternalId) to use calendarExternalId
instead so the spelling is consistent.
In `@packages/features/bookings/lib/handleCancelBooking.ts`:
- Around line 418-483: The recurring-delete loop in handleCancelBooking uses
bookingToDelete.user.credentials.filter(credential =>
credential.type.endsWith("_calendar")) and then for each credential finds each
updatedBooking's bookingRef but does not ensure the credential actually owns
that bookingRef; update the logic in the recurring-event branch (the Promise map
over credential) to, for each updBooking, find its bookingRef and then select
the matching credential by comparing bookingRef.credentialId (or falling back to
finding the credential in DB like the earlier credential lookup) before calling
getCalendar(...) and calendar.deleteEvent(uid,...); this ensures you only call
deleteEvent with the correct credential for each bookingRef and avoids
duplicate/failed deletes.
In `@packages/features/bookings/lib/handleNewBooking.ts`:
- Around line 1077-1079: The team destination calendars are dropped because
evt.destinationCalendar can be null and the optional chaining on
evt.destinationCalendar?.push(...) no-ops; change the logic in the COLLECTIVE
branch (where isTeamEventType and eventType.schedulingType === "COLLECTIVE") to
ensure evt.destinationCalendar is initialized to an array (e.g., from
eventType.destinationCalendar or organizerUser.destinationCalendar if present,
otherwise an empty array) before pushing or merging teamDestinationCalendars so
teamDestinationCalendars are preserved; update the code around
evt.destinationCalendar and the push/merge so it always appends/merges into a
real array.
- Around line 724-763: In loadUsers, remove the dead Array.isArray(hosts) guard
(hosts is set to eventType.hosts || []) and instead validate eventType.hosts
existence/type directly (e.g., check eventType.hosts is present and isArray)
before assignment; and fix error handling so HttpError instances are not
re-wrapped with a fixed 400—preserve and rethrow the original HttpError (or
construct a new HttpError forwarding error.statusCode/message when wrapping),
keep special handling for Prisma.PrismaClientKnownRequestError as appropriate,
and ensure dynamicUserList validation remains for the dynamic branch.
In `@packages/types/Calendar.d.ts`:
- Line 221: The interface change added a second parameter credentialId to
createEvent(event: CalendarEvent, credentialId: number):
Promise<NewCalendarEventType>, but most implementations still use the old
single-arg signature; update each concrete implementation (e.g.,
BaseCalendarService.createEvent, ZohoCrmCalendarService.createEvent,
BiginCalendarService.createEvent, SalesforceCalendarService.createEvent,
Office365CalendarService.createEvent, ExchangeCalendarService.createEvent
(exchange2016calendar/exchangecalendar/exchange2013calendar),
HubspotCalendarService.createEvent, LarkCalendarService.createEvent,
CloseComCalendarService.createEvent (sendgrid/closecom),
BasecampCalendarService.createEvent) to accept the new credentialId parameter,
propagate it through any internal calls or adapters, and ensure the returned
Promise<NewCalendarEventType> behavior remains unchanged.
🧹 Nitpick comments (3)
packages/core/EventManager.ts (1)
338-407: Multi-destination calendar creation logic looks correct.The flow properly handles three cases: credential-based lookup (with DB fallback), integration-type-based matching, and the no-destination fallback. One note: the DB fallback on Lines 344-367 constructs a
CredentialWithAppNamemanually — ensure theappNamefield (set tocredentialFromDB?.app.slug) is adequate since other places may expect the full app display name rather than the slug.Consider extracting the credential-fetching-from-DB logic (Lines 344-367) into a shared helper, as the exact same pattern is duplicated in
updateAllCalendarEvents(Lines 517-540).Example helper extraction
private async fetchCredentialById(credentialId: number): Promise<CredentialWithAppName | undefined> { const credentialFromDB = await prisma.credential.findUnique({ include: { app: { select: { slug: true } } }, where: { id: credentialId }, }); if (credentialFromDB && credentialFromDB.app?.slug) { return { appName: credentialFromDB.app.slug, id: credentialFromDB.id, type: credentialFromDB.type, key: credentialFromDB.key, userId: credentialFromDB.userId, teamId: credentialFromDB.teamId, invalid: credentialFromDB.invalid, appId: credentialFromDB.appId, }; } return undefined; }packages/features/bookings/lib/handleNewBooking.ts (2)
688-689: Consider using theSchedulingTypeenum instead of string literals for type safety.
SchedulingTypeis imported on line 71 but this check (and others at lines 992, 1021, 1077) uses raw strings"COLLECTIVE"/"ROUND_ROBIN". Using the enum values guards against typos and makes refactoring safer.♻️ Suggested change
const isTeamEventType = - !!eventType.schedulingType && ["COLLECTIVE", "ROUND_ROBIN"].includes(eventType.schedulingType); + !!eventType.schedulingType && + ([SchedulingType.COLLECTIVE, SchedulingType.ROUND_ROBIN] as string[]).includes(eventType.schedulingType);
987-1006: Side-effect inside.map()is a minor code smell.
teamDestinationCalendars.push(...)inside the.map()callback (line 993) mixes data collection with transformation. It works becausePromise.allis awaited on line 1008, but separating concerns would be clearer.
| @@ -246,7 +246,7 @@ test.describe("BOOKING_REJECTED", async () => { | |||
| }, | |||
| ], | |||
| location: "[redacted/dynamic]", | |||
| destinationCalendar: null, | |||
| destinationCalendar: [], | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the webhook.e2e.ts file at the mentioned lines
cat -n apps/web/playwright/webhook.e2e.ts | head -400 | tail -160Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 6096
🏁 Script executed:
# View the beginning of the webhook.e2e.ts file to find BOOKING_CREATED test
head -200 apps/web/playwright/webhook.e2e.ts | tail -100Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 3196
🏁 Script executed:
# Find handlers related to bookings
fd -t f 'confirm.handler' --glob '**' | head -5
fd -t f 'handleNewBooking' --glob '**' | head -5
# Search for where destinationCalendar is set in handlers
rg -l 'destinationCalendar' --type ts --glob '**/handlers' --glob '**/api' | head -10Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 418
🏁 Script executed:
# Find files related to booking handlers
find . -type f -name '*confirm*handler*' 2>/dev/null
find . -type f -name '*handleNewBooking*' 2>/dev/null
# Search for destinationCalendar in booking-related code
rg 'destinationCalendar' --type ts -l | grep -E '(booking|handler|webhook)' | head -15Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1188
🏁 Script executed:
# Examine the confirm handler (BOOKING_REJECTED path)
rg 'destinationCalendar' packages/trpc/server/routers/viewer/bookings/confirm.handler.ts -B3 -A3
# Examine handleNewBooking (BOOKING_CREATED and BOOKING_REQUESTED paths)
rg 'destinationCalendar' packages/features/bookings/lib/handleNewBooking.ts -B3 -A3Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 2189
The handlers intentionally produce different types for destinationCalendar.
The test expectations reflect the actual handler behavior:
confirm.handler.ts(BOOKING_REJECTED) defaults to[]when no calendar is foundhandleNewBooking.ts(BOOKING_CREATED/BOOKING_REQUESTED) defaults tonull
While the tests correctly reflect their respective handlers, this type inconsistency across webhook events suggests a potential API contract issue. Consider whether all webhook events should produce the same shape for destinationCalendar (either all [] or all null) for consistency.
🤖 Prompt for AI Agents
In `@apps/web/playwright/webhook.e2e.ts` at line 249, The handlers return
inconsistent types for destinationCalendar: confirm.handler.ts
(BOOKING_REJECTED) returns [] while handleNewBooking.ts
(BOOKING_CREATED/BOOKING_REQUESTED) returns null; pick a single canonical shape
(either always array or always null), update both handlers (confirm.handler.ts
and handleNewBooking.ts) to return that shape for destinationCalendar
consistently, and adjust the tests in webhook.e2e.ts to expect the unified value
for all webhook event tests (update cases for BOOKING_REJECTED, BOOKING_CREATED,
BOOKING_REQUESTED accordingly).
| // Find in calEventRaw.destinationCalendar the one with the same credentialId | ||
|
|
||
| const selectedCalendar = calEventRaw.destinationCalendar?.find( | ||
| (cal) => cal.credentialId === credentialId | ||
| )?.externalId; | ||
|
|
||
| calendar.events.insert( | ||
| { | ||
| auth: myGoogleAuth, | ||
| calendarId: selectedCalendar, | ||
| calendarId: selectedCalendar || "primary", |
There was a problem hiding this comment.
calendarId in the subsequent patch call (Line 168) lacks the || "primary" fallback.
Line 154 correctly falls back to "primary" when no matching credential is found, but the patch call on Line 168 uses selectedCalendar directly — which could be undefined if calEventRaw.destinationCalendar is empty/null and no match is found.
Proposed fix
calendar.events.patch({
// Update the same event but this time we know the hangout link
- calendarId: selectedCalendar,
+ calendarId: selectedCalendar || "primary",
auth: myGoogleAuth,🤖 Prompt for AI Agents
In `@packages/app-store/googlecalendar/lib/CalendarService.ts` around lines 145 -
154, The patch call in calendar.events.patch uses selectedCalendar directly
which can be undefined; update the calendarId passed to calendar.events.patch
(in CalendarService.ts where calendar.events.patch is invoked) to use the same
fallback as the insert call by changing calendarId: selectedCalendar to
calendarId: selectedCalendar || "primary" so it defaults to "primary" when no
matching destinationCalendar entry exists.
| const selectedCalendar = externalCalendarId | ||
| ? externalCalendarId | ||
| : event.destinationCalendar?.externalId; | ||
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; |
There was a problem hiding this comment.
Bug: tautological find — the else-branch always returns undefined.
When externalCalendarId is falsy, the code searches for cal.externalId === externalCalendarId, which compares against an undefined/null/empty value. This will never find a meaningful calendar entry, so selectedCalendar will always be undefined in the else-branch, causing the calendar.events.update call to receive undefined for calendarId.
The likely intent was to look up by credentialId or use the first destination calendar as a fallback.
Possible fix (mirror the createEvent pattern)
- const selectedCalendar = externalCalendarId
- ? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ const selectedCalendar = externalCalendarId
+ ? externalCalendarId
+ : event.destinationCalendar?.[0]?.externalId || "primary";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const selectedCalendar = externalCalendarId | |
| ? externalCalendarId | |
| : event.destinationCalendar?.externalId; | |
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; | |
| const selectedCalendar = externalCalendarId | |
| ? externalCalendarId | |
| : event.destinationCalendar?.[0]?.externalId || "primary"; |
🤖 Prompt for AI Agents
In `@packages/app-store/googlecalendar/lib/CalendarService.ts` around lines 254 -
256, selectedCalendar is being computed tautologically by comparing
cal.externalId to externalCalendarId, so when externalCalendarId is falsy the
find always returns undefined and calendar.events.update gets undefined; change
the else-branch to locate the externalId from event.destinationCalendar (e.g.,
find by credentialId or fall back to the first destinationCalendar's externalId)
— update the expression that assigns selectedCalendar (and any related logic
used later in calendar.events.update) to mirror the createEvent pattern: use
externalCalendarId if present, otherwise use event.destinationCalendar.find(cal
=> cal.credentialId === credentialId)?.externalId ??
event.destinationCalendar[0]?.externalId.
| const calendarId = externalCalendarId | ||
| ? externalCalendarId | ||
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; |
There was a problem hiding this comment.
Same tautological find bug as in updateEvent.
Identical issue: when externalCalendarId is falsy, searching for cal.externalId === externalCalendarId is a no-op. The defaultCalendarId fallback on Line 322 mitigates the impact here (unlike in updateEvent), but the find logic is still dead code.
Proposed fix
const calendarId = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : event.destinationCalendar?.[0]?.externalId;🤖 Prompt for AI Agents
In `@packages/app-store/googlecalendar/lib/CalendarService.ts` around lines 315 -
317, The current assignment to calendarId uses a tautological find: when
externalCalendarId is falsy the predicate cal.externalId === externalCalendarId
can never match, making the find dead code; change the logic in CalendarService
where calendarId is computed so that if externalCalendarId is provided use it,
otherwise select a fallback by searching event.destinationCalendar for a
suitable default (e.g., first calendar or one matching another criterion) before
falling back to defaultCalendarId—update the expression that references
externalCalendarId, event.destinationCalendar?.find(...), and defaultCalendarId
to perform the find only when externalCalendarId is falsy and the list exists.
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one | ||
| const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; | ||
| if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") { | ||
| evt["location"] = "integrations:daily"; | ||
| } |
There was a problem hiding this comment.
Null-pointer crash: mainHostDestinationCalendar can be undefined.
Line 118 destructures from evt.destinationCalendar ?? []. If the array is empty (or destinationCalendar is null/undefined), mainHostDestinationCalendar will be undefined. Line 119 then accesses .integration without optional chaining, which will throw a TypeError at runtime.
Proposed fix
const [mainHostDestinationCalendar] = evt.destinationCalendar ?? [];
- if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") {
+ if (evt.location === MeetLocationType && mainHostDestinationCalendar?.integration !== "google_calendar") {Note: With optional chaining, if mainHostDestinationCalendar is undefined, the expression evaluates to undefined !== "google_calendar" → true, which means it will fall back to daily video. Verify this is the desired behavior (i.e., when there's no destination calendar, Google Meet should not be used).
🤖 Prompt for AI Agents
In `@packages/core/EventManager.ts` around lines 117 - 121, The code destructures
mainHostDestinationCalendar from evt.destinationCalendar and then accesses
mainHostDestinationCalendar.integration without a null check, causing a
potential TypeError; update the condition in EventManager (where evt,
destinationCalendar, mainHostDestinationCalendar, and MeetLocationType are used)
to guard for undefined by using optional chaining or an explicit check (e.g.,
mainHostDestinationCalendar?.integration !== "google_calendar" or
mainHostDestinationCalendar && mainHostDestinationCalendar.integration !==
"google_calendar") so that when destinationCalendar is empty or undefined it
safely falls back to the daily integration.
| for (const reference of calendarReference) { | ||
| const { uid: bookingRefUid, externalCalendarId: bookingExternalCalendarId } = reference; | ||
| let calenderExternalId: string | null = null; | ||
| if (bookingExternalCalendarId) { | ||
| calenderExternalId = bookingExternalCalendarId; | ||
| } | ||
|
|
There was a problem hiding this comment.
Typo: calenderExternalId should be calendarExternalId.
This is in newly added code.
Proposed fix
- let calenderExternalId: string | null = null;
- if (bookingExternalCalendarId) {
- calenderExternalId = bookingExternalCalendarId;
- }
+ let calendarExternalId: string | null = null;
+ if (bookingExternalCalendarId) {
+ calendarExternalId = bookingExternalCalendarId;
+ }And update the usages on Lines 542 and 548 accordingly.
🤖 Prompt for AI Agents
In `@packages/core/EventManager.ts` around lines 504 - 510, Rename the misspelled
local variable calenderExternalId to calendarExternalId inside the loop that
iterates over calendarReference (the block that destructures bookingRefUid and
bookingExternalCalendarId) and update all subsequent usages of that variable
within the same function (every place that currently reads or assigns
calenderExternalId) to use calendarExternalId instead so the spelling is
consistent.
| if (credentialId) { | ||
| // Find the correct calendar credential under user credentials | ||
| const calendarCredential = bookingToDelete.user.credentials.find( | ||
| (credential) => credential.id === credentialId | ||
| ); | ||
| if (calendarCredential) { | ||
| const calendar = await getCalendar(calendarCredential); | ||
| if ( | ||
| bookingToDelete.eventType?.recurringEvent && | ||
| bookingToDelete.recurringEventId && | ||
| allRemainingBookings | ||
| ) { | ||
| const promises = bookingToDelete.user.credentials | ||
| .filter((credential) => credential.type.endsWith("_calendar")) | ||
| .map(async (credential) => { | ||
| const calendar = await getCalendar(credential); | ||
| for (const updBooking of updatedBookings) { | ||
| const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar")); | ||
| if (bookingRef) { | ||
| const { uid, externalCalendarId } = bookingRef; | ||
| const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId); | ||
| apiDeletes.push(deletedEvent); | ||
| if (bookingCalendarReference.length > 0) { | ||
| for (const reference of bookingCalendarReference) { | ||
| const { credentialId, uid, externalCalendarId } = reference; | ||
| // If the booking calendar reference contains a credentialId | ||
| if (credentialId) { | ||
| // Find the correct calendar credential under user credentials | ||
| let calendarCredential = bookingToDelete.user.credentials.find( | ||
| (credential) => credential.id === credentialId | ||
| ); | ||
| if (!calendarCredential) { | ||
| // get credential from DB | ||
| const foundCalendarCredential = await prisma.credential.findUnique({ | ||
| where: { | ||
| id: credentialId, | ||
| }, | ||
| }); | ||
| if (foundCalendarCredential) { | ||
| calendarCredential = foundCalendarCredential; | ||
| } | ||
| } | ||
| if (calendarCredential) { | ||
| const calendar = await getCalendar(calendarCredential); | ||
| if ( | ||
| bookingToDelete.eventType?.recurringEvent && | ||
| bookingToDelete.recurringEventId && | ||
| allRemainingBookings | ||
| ) { | ||
| const promises = bookingToDelete.user.credentials | ||
| .filter((credential) => credential.type.endsWith("_calendar")) | ||
| .map(async (credential) => { | ||
| const calendar = await getCalendar(credential); | ||
| for (const updBooking of updatedBookings) { | ||
| const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar")); | ||
| if (bookingRef) { | ||
| const { uid, externalCalendarId } = bookingRef; | ||
| const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId); | ||
| apiDeletes.push(deletedEvent); | ||
| } | ||
| } | ||
| }); | ||
| try { | ||
| await Promise.all(promises); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| logger.error(error.message); | ||
| } | ||
| }); | ||
| try { | ||
| await Promise.all(promises); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| logger.error(error.message); | ||
| } | ||
| } else { | ||
| apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); | ||
| } | ||
| } else { | ||
| } | ||
| } else { | ||
| // For bookings made before the refactor we go through the old behavior of running through each calendar credential | ||
| const calendarCredentials = bookingToDelete.user.credentials.filter((credential) => | ||
| credential.type.endsWith("_calendar") | ||
| ); | ||
| for (const credential of calendarCredentials) { | ||
| const calendar = await getCalendar(credential); | ||
| apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); | ||
| } | ||
| } | ||
| } else { | ||
| // For bookings made before the refactor we go through the old behaviour of running through each calendar credential | ||
| const calendarCredentials = bookingToDelete.user.credentials.filter((credential) => | ||
| credential.type.endsWith("_calendar") | ||
| ); | ||
| for (const credential of calendarCredentials) { | ||
| const calendar = await getCalendar(credential); | ||
| apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); | ||
| } | ||
| } |
There was a problem hiding this comment.
Recurring-event deletion path iterates over all calendar credentials rather than matching by reference.
Lines 449-460: When cancelling all remaining recurring bookings, the code iterates over all user calendar credentials (credential.type.endsWith("_calendar")), but each updated booking's reference (Line 454) is looked up only by type.includes("_calendar") and then deleted via whichever credential is currently iterating. This means:
- Each booking reference is found once per credential iteration, potentially causing duplicate delete attempts for the same event UID across different calendar credentials.
- If a credential doesn't own the event, the delete call will likely fail silently or throw.
If multi-calendar deletion is intentional (e.g., collective events on multiple hosts' calendars), consider matching the reference's credentialId to the current credential to avoid redundant API calls and errors.
Suggested improvement: match credential to reference
const promises = bookingToDelete.user.credentials
.filter((credential) => credential.type.endsWith("_calendar"))
.map(async (credential) => {
const calendar = await getCalendar(credential);
for (const updBooking of updatedBookings) {
- const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar"));
+ const bookingRef = updBooking.references.find(
+ (ref) => ref.type.includes("_calendar") && ref.credentialId === credential.id
+ );
if (bookingRef) {
const { uid, externalCalendarId } = bookingRef;
const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId);
apiDeletes.push(deletedEvent);
}
}
});🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleCancelBooking.ts` around lines 418 -
483, The recurring-delete loop in handleCancelBooking uses
bookingToDelete.user.credentials.filter(credential =>
credential.type.endsWith("_calendar")) and then for each credential finds each
updatedBooking's bookingRef but does not ensure the credential actually owns
that bookingRef; update the logic in the recurring-event branch (the Promise map
over credential) to, for each updBooking, find its bookingRef and then select
the matching credential by comparing bookingRef.credentialId (or falling back to
finding the credential in DB like the earlier credential lookup) before calling
getCalendar(...) and calendar.deleteEvent(uid,...); this ensures you only call
deleteEvent with the correct credential for each bookingRef and avoids
duplicate/failed deletes.
| const loadUsers = async () => { | ||
| try { | ||
| if (!eventTypeId) { | ||
| if (!Array.isArray(dynamicUserList) || dynamicUserList.length === 0) { | ||
| throw new Error("dynamicUserList is not properly defined or empty."); | ||
| } | ||
|
|
||
| const users = await prisma.user.findMany({ | ||
| where: { | ||
| username: { | ||
| in: dynamicUserList, | ||
| }, | ||
| username: { in: dynamicUserList }, | ||
| }, | ||
| select: { | ||
| ...userSelect.select, | ||
| credentials: true, // Don't leak to client | ||
| credentials: true, | ||
| metadata: true, | ||
| organization: { | ||
| select: { | ||
| slug: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| : eventType.hosts?.length | ||
| ? eventType.hosts.map(({ user, isFixed }) => ({ | ||
| }); | ||
|
|
||
| return users; | ||
| } else { | ||
| const hosts = eventType.hosts || []; | ||
|
|
||
| if (!Array.isArray(hosts)) { | ||
| throw new Error("eventType.hosts is not properly defined."); | ||
| } | ||
|
|
||
| const users = hosts.map(({ user, isFixed }) => ({ | ||
| ...user, | ||
| isFixed, | ||
| })) | ||
| : eventType.users || []; | ||
| })); | ||
|
|
||
| return users.length ? users : eventType.users; | ||
| } | ||
| } catch (error) { | ||
| if (error instanceof HttpError || error instanceof Prisma.PrismaClientKnownRequestError) { | ||
| throw new HttpError({ statusCode: 400, message: error.message }); | ||
| } | ||
| throw new HttpError({ statusCode: 500, message: "Unable to load users" }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Two issues in loadUsers: dead guard and swallowed HTTP status codes.
-
Dead code (Line 746):
hostsis assignedeventType.hosts || []on line 744, so!Array.isArray(hosts)is alwaysfalse. This guard never triggers. -
Lost status codes (Line 758-759): Re-wrapping an
HttpErrorasHttpError({ statusCode: 400 })discards the original status code. A 404 or 500 from an inner call becomes a misleading 400.
♻️ Suggested fix
} catch (error) {
- if (error instanceof HttpError || error instanceof Prisma.PrismaClientKnownRequestError) {
- throw new HttpError({ statusCode: 400, message: error.message });
+ if (error instanceof HttpError) {
+ throw error;
+ }
+ if (error instanceof Prisma.PrismaClientKnownRequestError) {
+ throw new HttpError({ statusCode: 400, message: error.message });
}
throw new HttpError({ statusCode: 500, message: "Unable to load users" });
}🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleNewBooking.ts` around lines 724 - 763,
In loadUsers, remove the dead Array.isArray(hosts) guard (hosts is set to
eventType.hosts || []) and instead validate eventType.hosts existence/type
directly (e.g., check eventType.hosts is present and isArray) before assignment;
and fix error handling so HttpError instances are not re-wrapped with a fixed
400—preserve and rethrow the original HttpError (or construct a new HttpError
forwarding error.statusCode/message when wrapping), keep special handling for
Prisma.PrismaClientKnownRequestError as appropriate, and ensure dynamicUserList
validation remains for the dynamic branch.
| if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") { | ||
| evt.destinationCalendar?.push(...teamDestinationCalendars); | ||
| } |
There was a problem hiding this comment.
Bug: Team destination calendars silently dropped when organizer has no destination calendar.
When neither eventType.destinationCalendar nor organizerUser.destinationCalendar is set, evt.destinationCalendar is null (line 1067). The optional chaining ?.push(...) on line 1078 then no-ops, and teamDestinationCalendars collected from COLLECTIVE team members are silently lost.
This means calendar events won't be created for team members who have destination calendars configured, if the organizer/event type doesn't have one.
🐛 Proposed fix
if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") {
- evt.destinationCalendar?.push(...teamDestinationCalendars);
+ if (teamDestinationCalendars.length > 0) {
+ if (!evt.destinationCalendar) {
+ evt.destinationCalendar = [];
+ }
+ evt.destinationCalendar.push(...teamDestinationCalendars);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") { | |
| evt.destinationCalendar?.push(...teamDestinationCalendars); | |
| } | |
| if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") { | |
| if (teamDestinationCalendars.length > 0) { | |
| if (!evt.destinationCalendar) { | |
| evt.destinationCalendar = []; | |
| } | |
| evt.destinationCalendar.push(...teamDestinationCalendars); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleNewBooking.ts` around lines 1077 - 1079,
The team destination calendars are dropped because evt.destinationCalendar can
be null and the optional chaining on evt.destinationCalendar?.push(...) no-ops;
change the logic in the COLLECTIVE branch (where isTeamEventType and
eventType.schedulingType === "COLLECTIVE") to ensure evt.destinationCalendar is
initialized to an array (e.g., from eventType.destinationCalendar or
organizerUser.destinationCalendar if present, otherwise an empty array) before
pushing or merging teamDestinationCalendars so teamDestinationCalendars are
preserved; update the code around evt.destinationCalendar and the push/merge so
it always appends/merges into a real array.
| @@ -216,7 +218,7 @@ export interface IntegrationCalendar extends Ensure<Partial<SelectedCalendar>, " | |||
| } | |||
|
|
|||
| export interface Calendar { | |||
| createEvent(event: CalendarEvent): Promise<NewCalendarEventType>; | |||
| createEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType>; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all classes implementing Calendar interface
rg -nP 'implements\s+Calendar' --type=ts -g '!node_modules' | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1917
🏁 Script executed:
#!/bin/bash
# Search for createEvent method signatures in all Calendar implementations
files=(
"packages/lib/CalendarService.ts"
"packages/app-store/zohocrm/lib/CalendarService.ts"
"packages/app-store/zoho-bigin/lib/CalendarService.ts"
"packages/app-store/sendgrid/lib/CalendarService.ts"
"packages/app-store/salesforce/lib/CalendarService.ts"
"packages/app-store/office365calendar/lib/CalendarService.ts"
"packages/app-store/exchange2016calendar/lib/CalendarService.ts"
"packages/app-store/hubspot/lib/CalendarService.ts"
"packages/app-store/larkcalendar/lib/CalendarService.ts"
"packages/app-store/googlecalendar/lib/CalendarService.ts"
"packages/app-store/exchangecalendar/lib/CalendarService.ts"
"packages/app-store/exchange2013calendar/lib/CalendarService.ts"
"packages/app-store/closecom/lib/CalendarService.ts"
"packages/app-store/basecamp3/lib/CalendarService.ts"
)
for file in "${files[@]}"; do
if [ -f "$file" ]; then
echo "=== $file ==="
rg -A2 'createEvent\s*\(' "$file" | head -5
fi
doneRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 3306
Breaking interface change: 13 of 14 Calendar implementations have not been updated with the credentialId parameter.
The interface was updated to require credentialId in the createEvent method signature, but only GoogleCalendarService has been updated. The following implementations are missing this parameter and will cause type mismatches:
BaseCalendarServiceZohoCrmCalendarServiceBiginCalendarServiceSalesforceCalendarServiceOffice365CalendarServiceExchangeCalendarService(exchange2016calendar, exchangecalendar, exchange2013calendar)HubspotCalendarServiceLarkCalendarServiceCloseComCalendarService(sendgrid, closecom)BasecampCalendarService
All these implementations must be updated to match the new interface signature.
🤖 Prompt for AI Agents
In `@packages/types/Calendar.d.ts` at line 221, The interface change added a
second parameter credentialId to createEvent(event: CalendarEvent, credentialId:
number): Promise<NewCalendarEventType>, but most implementations still use the
old single-arg signature; update each concrete implementation (e.g.,
BaseCalendarService.createEvent, ZohoCrmCalendarService.createEvent,
BiginCalendarService.createEvent, SalesforceCalendarService.createEvent,
Office365CalendarService.createEvent, ExchangeCalendarService.createEvent
(exchange2016calendar/exchangecalendar/exchange2013calendar),
HubspotCalendarService.createEvent, LarkCalendarService.createEvent,
CloseComCalendarService.createEvent (sendgrid/closecom),
BasecampCalendarService.createEvent) to accept the new credentialId parameter,
propagate it through any internal calls or adapters, and ensure the returned
Promise<NewCalendarEventType> behavior remains unchanged.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements